Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add registry option to download commands #893

Conversation

uncleDecart
Copy link
Member

New eden workflows take eve registry and tag as a parameter and set it up as default. During eve upgrade testing eden utils download eve-rootfs is called which is using default registry, however intended behaviour would be to use lf-edge/eve registry. Adding this parameter and default value makes download util use lf-edge/eve registry by default.

This does not break behaviour in workflows

@giggsoff
Copy link
Collaborator

Can you please help me to understand, do you plan to set different global eve.registry option in config, but keep lf-edge/eve in eve_update tests? Do you plan to adjust tests somehow?

@uncleDecart
Copy link
Member Author

@giggsoff eve-update tests are using eden utils download command to download eve 8.7.0 here, which uses registry from config. And if we add registry parameter to cli, which overrides that and set default value to lf-edge/eve we always will download eve images from lf-edge by default. That way test file won't change, but its behaviour will

@giggsoff
Copy link
Collaborator

giggsoff commented Sep 29, 2023

Seems it does not work (checked your branch):

$ make build
$ ./eden config add
$ ./eden config set default --key=eve.registry --value="giggsoff/eve"
$ ./eden utils download eve-rootfs --eve-tag=8.6.0 --downloader-dist=$(pwd)/dist
FATA[0001] ImagePull (giggsoff/eve:8.6.0-kvm-amd64): imagePull: Error response from daemon: manifest for giggsoff/eve:8.6.0-kvm-amd64 not found: manifest unknown: manifest unknown

@uncleDecart uncleDecart force-pushed the add-registry-option-to-download-command branch from 9b0e996 to e6d7504 Compare September 29, 2023 08:28
@uncleDecart
Copy link
Member Author

@giggsoff you're right, using set default overrides default values set in flags by Merge function. I updated http test accordingly (and bumped eve version to 8.7.0 in OCI test, although I think we can bump it to latest LTS 9.4.5)

@giggsoff
Copy link
Collaborator

giggsoff commented Sep 29, 2023

Please update both tests (http and oci) using novel registry option (I mean move it on top of file the same way you do it for http test). And use different versions of EVE-OS in tests as they will not re-downloaded if exists in different partition (not sure how it works now, but it is separate question, we do not plan to check it here).

@uncleDecart
Copy link
Member Author

Please update both tests (http and oci) using novel registry option (I mean move it on top of file the same way you do it for http test). And use different versions of EVE-OS in tests as they will not re-downloaded if exists in different partition (not sure how it works now, but it is separate question, we do not plan to check it here).

@giggsoff but oci test is not using utils download it directly tries to download from lf-edge/eve oci registry, so I don't see the point of updating it. Will bump EVE version though

@uncleDecart uncleDecart force-pushed the add-registry-option-to-download-command branch from e6d7504 to 272ee58 Compare September 29, 2023 13:05
@giggsoff
Copy link
Collaborator

Please update both tests (http and oci) using novel registry option (I mean move it on top of file the same way you do it for http test). And use different versions of EVE-OS in tests as they will not re-downloaded if exists in different partition (not sure how it works now, but it is separate question, we do not plan to check it here).

@giggsoff but oci test is not using utils download it directly tries to download from lf-edge/eve oci registry, so I don't see the point of updating it. Will bump EVE version though

Two points:

  1. You put variable on top of http test, but the same option is hardcoded in oci. The best way is to allow user to configure it for example using some environment variable. But at least it would be great to see eve_registry on top of oci in the same place as for http
  2. We should use different versions of EVE-OS in oci and http tests (and the third one is inside defaults or global config). We want to check the whole update mechanism, but in case of the same versions potentially EVE-OS will not install anything, but just reboot with another partiton where we install rootfs in previous test.

New eden workflows take eve registry and tag as a parameter
and set it up as default. During eve upgrade testing
`eden utils download eve-rootfs` is called which is using
default registry, however intended behaviour would be to
use `lf-edge/eve` registry. Adding this parameter and
setting it in upgrade test makes download util use
lf-edge/eve registry

Signed-off-by: Pavel Abramov <[email protected]>
@uncleDecart uncleDecart force-pushed the add-registry-option-to-download-command branch from 272ee58 to d333713 Compare September 29, 2023 13:58
@uncleDecart
Copy link
Member Author

@giggsoff updated PR to use registry for OCI from variables and use two different EVE versions in OCI and HTTP tests

@uncleDecart uncleDecart merged commit b8d8adb into lf-edge:master Oct 2, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants